Allow a metric to be unlisted - #13616

Thanks — all three findings were real, including the two that were filed as suppressed comments. Fixed in 9ed3ee8.

tombstone() / tombstoned() accepting ids that name no allocated slot. The bound check was blob_ix == _cur_blob && offset > _cur_off, which only constrains the current blob. For any blob below it the offset is the low 16 bits of the id and so can reach 65535 against a 1024-entry array, and tombstone() writes through it. Both now go through a new Storage::allocated():

bool
allocated(IdType id) const
{
  auto [blob, entry] = _splitID(id);

  if (id < 0 || entry >= MAX_SIZE || !_blobs[blob]) {
    return false;
  }

  return blob < _cur_blob || (blob == _cur_blob && entry < _cur_off);
}

This is deliberately stricter than the existing valid() in the second way you noted as well: _cur_off is the next free slot, so valid() accepts one slot that does not exist yet. That mattered more than I expected. A new test marked the free slot, and an unrelated metric created in a later test section landed in it and came out invisible, because create() only clears the flag when it finds the name already present, not when it allocates a fresh slot. So the "harmless" case was a real way to lose a metric.

I left valid() alone rather than tightening it here — other callers depend on its current semantics and that is a separate change.

The positional iterator constructor. You are right that it let a caller rest an iterator on a tombstoned slot, which reintroduces the non-terminating range-walk. Rather than only skipping, the three constructors are now private with friend class Metrics, so begin(), end() and find() are the only ways to obtain one. find() already resolves a tombstoned name to end(). The positional constructor also skips now, so the invariant holds for any future in-class use.

New tests covering each case: an offset past the end of a full blob, the next free slot, and a blob index that was never allocated.

Follow-up on a review question: allocated() indexes _blobs with a blob index taken from a caller-supplied id and does not range check it.

That is in fact safe, but only by coincidence. _splitID masks the blob index with METRIC_TYPE_MASK (0x1FFF), and MAX_BLOBS is 8192, so the index is always a valid _blobs subscript by construction rather than by a check. Nothing in the code tied those two constants together, and every other accessor that splits an id — lookup, name, rename, valid — depends on the same relationship.

b4fee22 makes it explicit:

static_assert(MAX_BLOBS == METRIC_TYPE_MASK + 1, "a masked blob index must always be a valid _blobs index");

Verified it fires: dropping MAX_BLOBS to 4096 fails the build rather than silently producing out-of-range subscripts throughout the class.

Also added a test for the largest possible id, which exercises the other half — the offset is not masked to the blob size, so it needs the explicit entry >= MAX_SIZE check to avoid running off the end of a blob.

Worth noting for a possible follow-up, out of scope here: METRIC_TYPE_MASK is misnamed. It has exactly one use, masking the blob index in _splitID, and has nothing to do with the metric type, which lives at METRIC_TYPE_BITS. Renaming it would be a one-line change but it is a public constant, so I left it alone.

Went through all six. Two were already addressed, three are the same point and are now fixed, and one is incorrect — details below.

Fixed in ef97aa6

create() reimplementing the flag clear, and the duplicated ~UNLISTED mask (three of the comments): create() now calls set_listed(it->second, true), so the mask appears in exactly one place and there is one code path for relisting. On the null-blob concern: a name present in _lookups always names an allocated slot, so neither the old inline check nor the new allocated() gate can fail there — but routing through one function means the failure modes cannot drift apart, which was the substance of the comment.

current() returning std::pair<int16_t, int16_t> built from two uint16_t members is now std::pair<uint16_t, uint16_t>. The values are small enough that the narrowing never mattered, but see below for why it was worth removing.

Not a defect: the negative-offset concern

entry comes from _splitID(id) as an int16_t (low 16 bits). If those low bits are >= 0x8000, entry becomes negative and will pass entry >= MAX_SIZE

_splitID returns std::tuple<uint16_t, uint16_t>, so entry is unsigned. Low bits of 0xFFFF give entry == 65535, which fails entry >= MAX_SIZE and is rejected. There is no signed value and no negative subscript. The existing test for std::numeric_limits<IdType>::max() covers exactly this input — its offset bits are 0xFFFF, high bit set — and asserts both unlist and listed return false.

The int16_t in the comment is real, though, just in a different function: current(), which is what I changed above. That is very likely where the reading came from, so removing the narrowing is worth it even though allocated() was never affected.

Already addressed

The Storage::tombstone() allocated-slot comment was fixed in 9ed3ee8, which is what introduced allocated().

The PR description mismatch was real when filed; the description now documents unlist/relist/listed and includes a section on why the name changed. No compatibility alias: tombstone was never released, and it is the wrong word for this — a tombstone marks something deleted and reusable, which is how CacheShm in this same tree uses it. Nothing is deleted here.

FlagStorage uses std::atomic<uint8_t>, whose default constructor does not initialize the stored value.

That was true through C++17, but P0883 changed std::atomic's default constructor to value-initialize the contained value in C++20, and this tree builds -std=c++20.

It also does not depend on that change. addBlob() allocates with std::make_unique<Metrics::NamesAndAtomics>(), which is new T() — value-initialization — and that recurses through the tuple and the std::array to every element. Under the pre-C++20 rules the atomic's defaulted default constructor was trivial, so value-initialization zero-initialized its storage anyway. Both readings give a zero flag byte, so a new slot starts listed.

I would rather not argue that from the standard, so 02d30f9 asserts it instead. The blob growth test already creates MAX_SIZE + 100 metrics, which spans a blob boundary; it now also requires listed(id) for every one of them, so a full blob's worth of freshly allocated slots is checked.

Confirmed the assertion is not vacuous. Storing 0xFF into the flag array immediately after the allocation fails it:

test_Metrics.cc:600: FAILED:
  REQUIRE( h.listed(id) )

That matters more than the standard argument, because reading uninitialized heap frequently does return zero — fresh pages are zero-filled — so this class of bug hides well and a test that only samples a metric or two would not catch it.

I did not add an explicit initialization loop. It would be dead work on every blob, and the real risk is not today's behavior but a future change to something like make_unique_for_overwrite; the test catches that, and addBlob() now says so at the allocation.

This one is a real bug, and mine. Fixed in 7b19905.

find() returned an iterator whose _it was the stored id, type bits and all, while _bound comes from current_id() and is built with COUNTER type bits. For a GAUGE the type bit at METRIC_TYPE_BITS puts _it above any realistic bound, so at_end() was immediately true and the iterator compared equal to end(). find() was therefore broken for every gauge in the store, not just unlisted ones. advance() already normalized the position; the positional constructor did not.

It is a regression from the bound comparison I introduced with unlisting — before that, operator== compared raw ids and the type bits cancelled out. No production code calls find(), so nothing was broken in the field, but every test I wrote for it happened to use a counter, which is why it got through.

Fixed as suggested, by keeping only the blob and offset:

auto [blob, offset] = _metrics._splitID(pos);

_it = _makeId(blob, offset, MetricType::COUNTER);

Dereferencing is unaffected: Storage::lookup(id, ...) deliberately reads the type from the slot rather than the id, precisely because iterators manufacture positions.

Test added first and watched fail on REQUIRE(g != m.end()), now covering both a gauge and a counter through find(), including that the dereferenced type comes back as GAUGE.

Separately, I have dropped the commit I added earlier about FlagStorage initialization. That comment was incorrect and a reply should have been the whole response; the extra commit was noise.

Correct as stated, but deliberately out of scope here.

allocated() reads _blobs[blob], _cur_blob and _cur_off without the mutex because that is what every other id-taking accessor in this class already does:

• Storage::valid() — the same two fields, unlocked
• Storage::lookup(IdType, ...) — !blob || (blob_ix == _cur_blob && offset > _cur_off), unlocked
• Storage::name(IdType) — the same guard, unlocked
• Storage::rename() — the same guard, before it takes the lock

So this is not a race introduced by unlisting; it is the existing synchronization model of Storage, and allocated() was written to match it rather than to invent a second convention in the same class. Making just this one function lock while its neighbours do not would be misleading about the guarantees, and taking _mutex in allocated() would also put a lock in the iterator's skip path, which is currently lock free by design.

The synchronization of this class is being addressed directly in #13583, "Metrics: close the id lookup race and bounds gaps left by the lock revert". That is the right place for it — it is a property of the whole store, not of this feature, and fixing it in two PRs at once would just produce conflicts.

Whichever of the two lands second should extend the fix to cover the other's accessors: if #13583 goes first, allocated() needs the same treatment; if this one goes first, #13583 picks it up along with valid(), lookup() and name().

Five comments, one real bug. Taking them in order of consequence.

The subrange across snapshots — real, fixed in e776b65

This is a genuine defect and reachable through the public API. Two find() calls with a metric created between them is enough:

auto start = m.find("a");        // captures bound B1
Metrics::Counter::create("z");   // store grows
auto stop  = m.find("z");        // bound B2 > B1, and its position is >= B1

for (auto it = start; it != stop; ++it) { ... }   // never terminates

it passes B1, becomes exhausted against its own bound, stop is still live against B2, so they never compare equal and operator++ cannot make progress. I wrote the test first, with a step cap so it fails rather than hanging, and watched it fail.

Fixed by judging exhaustion between two positional iterators against the earlier of the two bounds, so such a subrange ends at the earlier snapshot. The sentinel keeps its own answer, since its bound is meaningless — folding it into the minimum would make every other iterator compare exhausted immediately.

My earlier subrange test missed this because it created both endpoints after all the metrics, so both held the same bound.

The count > 0 coupling — fair, fixed in 1f0f4ca

Filed twice, and right both times: that assertion did depend on the shared store holding at least one listed metric. In practice slot 0 is always listed so it could not actually fail, but the coupling is real and pointless. The section now creates a listed metric of its own and asserts it is observed.

One correction to the suggestion: an anchor placed before the unlisted tail proves the loop ran, not that iteration reached the tail. The "none of the tail names appear" assertion is what covers the tail. The comment says only the former.

Structured bindings and -Werror — not an issue

The concern is that binding [name, type, value] and using only name trips an unused-variable warning under -Werror. Neither clang nor gcc warns per element of a structured binding, and I can show it rather than argue it: the exact translation unit compiles with

-Wall -Werror -Wextra -Wsuggest-override -Wthread-safety ...

and is clean. The pattern is also already in the tree — two occurrences in this same file before this PR, three in RecCore.cc. Rewriting to std::get<0>(entry) would diverge from that for no benefit.

listed() const-correctness — declining, but happy to be overruled

Correct that NamesAndAtomics const * would be tighter in a const method. I left it because lookup(IdType, ...) and name() immediately above it do exactly the same thing, and changing one of three makes the file less consistent, not more. It is a worthwhile sweep across all of them as its own change; say the word if you would rather I just do the one here.

Both commits build and pass test_tsutil individually.

Two to answer, since the FlagStorage one is a repeat and already addressed above.

Iterator equality is not an equivalence relation — correct, documented in 03452ff

The counterexample is real and I can be precise about it. With bounds B1 < B2 and an iterator a holding B1, b holding B2, and b positioned in [B1, B2): a == b (both past the shared minimum), a == end() (past its own bound), but b != end(). Transitivity fails.

The cause is structural rather than a slip in the comparison: exhaustion is a property of each iterator's own snapshot, so any iterator-versus-end test is per-iterator, and no comparison rule over a single type can paper over that.

I looked at the sentinel design you suggest. It does fix it properly — iterator-versus-iterator becomes pure position equality, iterator-versus-sentinel carries the end test, and the two are different relations so nothing is required to hold across them. The cost is that end()'s return type changes in an installed header and std::distance(m.begin(), m.end()) stops compiling; there are two such lines in this file already, predating this PR.

We decided against it for now and documented the limitation instead. These are input_iterator_tag, the only uses in tree are range-for over the whole store, std::distance(begin, end), and find() != end(), and none of them depends on transitivity. Trading a public type change for a property nothing relies on did not seem the right call inside this PR. The note on operator== now says plainly that only same snapshot comparisons and comparison against end() are meaningful, and that these should not be handed to a generic algorithm — so the next person meets the constraint at the point of use rather than deriving it. If you would rather have the sentinel, it is a contained change and I am happy to do it.

Worth adding: the min-bound rule this replaced a hang, not correct behaviour. Before it, that same subrange did not terminate at all.

Many SECTIONs mutating the singleton store — declining

The remedy does not do what it is meant to here. Splitting into separate TEST_CASEs would only reduce accumulation if the shared body registered metrics on each re-run, and this one does not:

TEST_CASE("Metrics unlisting", "[libtsapi][Metrics]")
{
  auto &m = Metrics::instance();

  SECTION(...)

The body is a reference binding. Every registration happens inside a SECTION, so it happens exactly once whichever way the cases are split, and the total is identical. Splitting also would not reset anything — it is the same process and the same singleton either way, and there is deliberately no reset hook, since the store never frees a slot.

On volume: the largest fill in this test case is 8 metrics, and it adds a few dozen in total against a store that holds 8M. The pattern of many sections over Metrics::instance() is also what the rest of this file already does.

The one place this was a real problem was an assertion that depended on the store holding at least one listed metric from elsewhere, which is fixed in 1f0f4ca.

Both of these arrived twice; answering once each.

input_iterator_tag versus the note — fair, and my note caused it. Reframed in 0d16232

The objection lands on the previous wording rather than on the code: declaring std::input_iterator_tag and then saying "not iterators to hand to a generic algorithm" advertises a contract and denies it in the same breath. That was mine and it was the wrong framing.

The framing that fits what the code does: a snapshot is the sequence. begin() fixes the extent to be walked; iterators from two different snapshots are no more comparable than iterators into two different containers, and the standard already treats that as outside the domain rather than as a broken relation. Within one snapshot every iterator shares a bound, so equality is exactly the equivalence relation an input iterator requires, and the tag is honest.

The min-bound rule then has a narrower job than the old note implied: it makes the out-of-domain case terminate instead of hang. Before it, that comparison did not terminate at all, which is a worse kind of unspecified.

On the two suggested remedies: the sentinel design does fix the relation properly and I priced it out — end()'s return type changes in an installed header and std::distance(m.begin(), m.end()) stops compiling, which is two lines in this file that predate the PR. We decided that was not a trade to make inside this PR. It remains a contained change if a maintainer prefers it.

Private iterator constructors are a source break — correct, and now documented

Accurate: iterator(const Metrics &, IdType) was public and is not any more.

I am keeping it private. Nothing in tree constructs an iterator directly, and there is no sensible reason to — a caller holding an id wants lookup, not an iterator positioned at one slot. More to the point, that public constructor is where both iterator defects found in this review came from: a gauge id whose type bits put it past the bound, and a subrange built from two snapshots that never terminated. Both required naming an arbitrary position. The suggested compromise, keeping it public but normalising unlisted positions to end(), closes neither.

What was missing was disclosure, so the PR description now has an API change section recording it, for the 11.0.0 release notes alongside the createSpan and rename removals from #13583.

Follow-up #13666 opens the consumer of unlist: the per server connection metrics retract their published names when metric_aggregate changes. It is a draft while this PR is open, since it carries this branch's commit until then.